PICARD-1001 Rework local cover art handling #1934

kepstin · 2021-10-30T21:56:50Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences: Rework local cover art handling from a cover art provider to a property of files.

Problem

Local cover art as a provider doesn't make sense - local cover art is a property of a file, same as tag cover art. Problems caused by local cover art being treated as a provider include incorrectly indicating that files have been modified, or if an album was loaded without any files associated (for example via a tagger link on musicbrainz.org), no cover art at all.

JIRA ticket (optional): PICARD-1001

Solution

Move the handling of local cover art to the File class and delete the Local File cover art provider

Action

I don't know how to write Qt UI, so someone familiar with that will have to update the UI for this change.

In the current state, the local_cover_regex setting is re-used, and a new load_local_cover_art boolean is used to enable the feature. The new setting should default to the same as the enabled status of the Local FIle cover art provider used to be.

Some decision has to be made on how to result ordering of loading cover art from tags vs. local files. Currently local file art loading is skipped if cover art was loaded from tags.

Local cover art as a provider doesn't make sense - local cover art is a property of a file, same as tag cover art. Move the handling of local cover art to the File class. TODO: Need to resolve how to configure load ordering of tag cover art vs local cover art. Also, there needs to be a config ui written. In the current state, the local_cover_regex setting is re-used, and a new load_local_cover_art boolean is used to enable the feature. This will probably fix many of the causes of PICARD-1001.

kepstin · 2021-10-30T22:40:53Z

(Pushed a minor updates, this adds the settings back that had disappeared when I deleted the provider ui code, and fixed initialization of the local file cover art object from url)

kepstin · 2021-10-30T22:44:13Z

Ah, looking into it, this doesn't actually solve the problem.

The issue I am having now is that when a release has a local file loaded, and also gets a cover image from CAA, it shows up as "changed" even if both images are the same image. I'm not sure what the solution to this is.

(in this example, top cover is from CAA provider, bottom one is loaded from local file)

How is this resolved for the case of images saved in tags? As far as I can tell, i'm using the same codepaths here - does that also fail to match?

kepstin · 2021-10-30T23:01:15Z

Looking at the contents of orig_metadata and metadata in the File.update method, I'm seeing this:

orig_metadata = ImageList([LocalFileCoverArtImage(url='file:///mnt/makoto/home/cwalton/Music/Flac/brainz-flac/ALI PROJECT/わが﨟たし悪の華/cover.jpg', types=['front'], support_types=True, support_multi_types=True)])
metadata = ImageList([CaaCoverArtImage(url='http://coverartarchive.org/release/81b330ce-62ee-4d9a-a2a7-6594204d3131/23095838320-500.jpg', types=['front', 'booklet'], support_types=True, support_multi_types=True, is_front=True)]

So I think what's needed here is a better equality or comparison method on the picard.util.ImageList or maybe picard.coverart.image.CoverArtImage implementation which can figure out that the two images are the same despite being different classes, so a save is not required.

kepstin · 2021-10-30T23:08:54Z

Interesting… took a look at the __eq__ method on CoverArtImage, and I see that the reason these two images aren't considered to match is that both are flagged as supports_multi_types = True, but the list of types is disjoint.

Either we need to have some special handling for the case of the frontiest image that ignores multiple types, or I can set supports_multi_types = False on the local files loader. (I'm not sure if that even works, anyways? Would require a custom regex with a capture that could match multiple times, and I don't think i saw code to handle that…)

I'll push the supports_multi_types = False change for the time being since it fixes the single cover.jpg case for now.

This fixes the comparison with CAA art when the CAA art has multiple types. (For example, comparing cover.jpg with a CAA "cover, booklet" image)

phw

First off, your observations are correct. For the reasons you stated "local cover art" never worked as it should and moving this away from being a cover art provider is definitely right.

But we must be aware that there are differences in behavior with this change. One is that Picard will overwrite the cover art with cover loaded from providers. Before that you could put local cover art provider to be the first one, and it would mean local cover art ist used and other providers not even called. I think we should do the following:

Have an option to "Do not overwrite existing images", which will keep the images loaded for a file even if different cover art has been loaded.
Keep the "Local Files" provider, but rename it to "Local Files (deprecated)". That way people relying on this can continue using this for now, and we get some time to see what users need. Alternative to this would be to put it in a plugin instead. But my preference is to keep it in the code for the next release.
Maybe we should have a settings kigration that nebales load_local_cover_art if the provider is active? But not sure about this.

Regarding the UI I can help out and do the necessary changes this evening. I can also do an actual code review then.

zas · 2021-10-31T11:07:27Z

This approach doesn't let the user set the order of preference for cover art sources.
I'm not sure the actual problem is the fact it's a "cover art provider".
We are well aware of limitations of cover art stuff (which was, at start, a hack), and I tried to work on this more than once, but the whole thing is very complex, probably much more you think it is.

Before any review, PR needs to pass tests.

Also I disagree with statement "local cover art" never worked as it should, because it was never defined how it should work to start with (or I'm not aware of such document).

That said, it's clear cover art code needs improvements and any patch is welcome.
Can we first define how the local cover art should work and what's broken?

kepstin · 2021-10-31T14:32:25Z

So, the way I think about local cover art files is that they should be treated equivalently to cover art embedded into tags in a file. Aligning the picard code to match that was the goal of this PR.

I agree that an option to preserve existing cover art would be desirable. Such an option would be applicable both to local cover art files and embedded cover art in tags.

The particular goals of this PR were to:

Have Picard update the local cover art files using images from configured cover art providers, in the same way as it does for images embedded in tags.
Have Picard indicate when it is updating local cover art files with images from cover art providers, with working image differences in the cover art panel.
Have Picard show the album as having no changes to save if the local cover art files match the cover art from the cover art provider.

phw · 2021-11-01T09:16:45Z

Also I disagree with statement "local cover art" never worked as it should, because it was never defined how it should work to start with (or I'm not aware of such document).

Agreed. The local cover art provider definitely did work for specific use cases. I think the main problem with it is that by design it can only run if files are attached to the release already when it gets loaded. That means it does not work for quite a few cases. But one use case worked quite well: I have the local cover art provider on top of my provider list. That way when I retag files the existing images are used. That means it remembers my choice of cover art, even if when tagging I chose a different cover art than what my primary provider would have given.

So, the way I think about local cover art files is that they should be treated equivalently to cover art embedded into tags in a file. Aligning the picard code to match that was the goal of this PR.

I fully agree with that. I often had the opposite desire, though: Have embedded images work the same as the local cover art provider.

I think we must provide some use cases which users might have with local cover art. Some that come to my mind:

Load local cover art for files the same as embedded images, so the user can compare them to the new images which have been loaded by providers.
If there is already cover art for the files (local or embedded) keep this and do not use cover art from the providers.
Use cover art from one preferred provider (e.g. CAA), but fall back to local files and only if there are no local files load from another fallback provider (e.g. Amazon).
Load local cover art files in order to embed them into the files.

I would claim that Picard currently does not support any of the above use cases well, and both this implementation and the current local cover art provider solve different use cases differently well.

Local cover art provider does not solve 1. It can solve 2, 3 and 4, but only under certain circumstances (files are available when a release gets loaded). It cannot solve 2, 3 and 4 for embedded cover art.

This PR does solve 1. It does not solve 2 or 3. It can be used for 4 given that there is no cover art loaded from providers.

If we add an option to prefer local or embedded files and not replace it with provider files (that means "Use original cover art" is the default behavior), and maybe also an option to toggle loading embedded images on and off then it would also solve 2 and 4. Not sure how it could solve 3 (which is based on provider order).

What did I forget?

phw · 2021-11-01T09:23:04Z

picard/file.py

@@ -250,6 +260,48 @@ def _loading_finished(self, callback, result=None, error=None):
        self.update()
        callback(self)

+    _types_split_re = re.compile('[^a-z0-9]', re.IGNORECASE)
+    _known_types = set([t['name'] for t in CAA_TYPES])
+    _default_types = ['front']


This could be written directly as {t['name'] for t in CAA_TYPES}

phw · 2021-11-01T09:25:32Z

picard/file.py

+    _default_types = ['front']
+
+    def get_types(self, string):
+        found = set([x.lower() for x in self._types_split_re.split(string) if x])


Can also be done with a set comprehension {x.lower() for x in self._types_split_re.split(string) if x}

phw · 2021-11-01T09:26:52Z

picard/coverart/image.py

@@ -480,3 +480,6 @@ def __init__(self, filepath, types=None, comment='',
        super().__init__(url=url, types=types, comment=comment)
        self.support_types = support_types
        self.support_multi_types = support_multi_types
+        path = self.url.toLocalFile()
+        with open(path, 'rb') as file:
+            self.set_data(file.read())


I think loading the data here makes sense, but this needs fixing a couple of tests that currently fail due to this. The LocalFileCoverArtImageTest currently works without files, should be updated to work on temporary files.

zas · 2021-11-01T09:33:31Z

You summed it up pretty well, I agree on all 4 cases.

First, we need to define what are local cover art files, they may have a way to determine types or not. Also they may not be at same directory level, I can imagine someone having following structure:

dir/
  file1.flac
  file2.flac
  coverart/
    front.jpg
    back.jpg

Also source image files can be named in infinite number of ways.

dir/
  file1.flac (with embedded track front cover)
  file2.flac (with embedded track front cover)
  track1.jpg
  track2.jpg
  folder.png
  booklet.pdf

We have a regex to match those, but I think it's insufficient. MB doesn't have track cover art support yet.
Local files type identification may be complex (or even impossible), or the type can be in the filename, but with a different language.
All this makes "merging" local cover art with remote cover art quite messy.

So, I think we need a way to "map" local cover art, to "remote" cover art, and decide how to handle multiple images of the same type.

kepstin · 2021-11-01T15:07:27Z

If we add an option to prefer local or embedded files and not replace it with provider files (that means "Use original cover art" is the default behavior), and maybe also an option to toggle loading embedded images on and off then it would also solve 2 and 4. Not sure how it could solve 3 (which is based on provider order).

What did I forget?

I think I pretty much agree with this - an option to preserve existing cover art would be a necessity before any change like this gets merged. The use case for 3 seems to be fairly niche? And yeah, that would be hard to do.

The particular concern I had was that there's no ability to set the relative priority of cover art from local file vs. embedded cover art right now. Adding a whole second priority list to the UI for this doesn't seem like a good idea, so I'm kind of stumped on that front.

… another possible option might be to have "cover art providers" actually have multiple possible hook points? So when a cover art provider registers itself, it could register itself to run at either the album_metadata_processor stage or the file_post_load_processors stage? (or both, i suppose). This would require some extra work to resolve (probably in the file metadata update function) the relative priorities of cover art from the file vs from album metadata, but it could work.

Any opinions on that?

If this is done, it might make sense to have tag cover art be in the providers list as well to allow setting its relative priority (even tho it's a special case, since I think the implementation for that might need to stay in the specific file format implementations)

zas requested a review from phw October 30, 2021 22:33

kepstin force-pushed the picard-1001 branch from 1224d68 to dbfe7f8 Compare October 30, 2021 22:38

Disable the 'supports multi types' on local cover art

37f2054

This fixes the comparison with CAA art when the CAA art has multiple types. (For example, comparing cover.jpg with a CAA "cover, booklet" image)

phw reviewed Oct 31, 2021

View reviewed changes

phw requested a review from zas November 1, 2021 09:20

phw added the Needs more work label Nov 1, 2021

phw requested changes Nov 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-1001 Rework local cover art handling #1934

PICARD-1001 Rework local cover art handling #1934

kepstin commented Oct 30, 2021 •

edited

Loading

kepstin commented Oct 30, 2021

kepstin commented Oct 30, 2021 •

edited

Loading

kepstin commented Oct 30, 2021

kepstin commented Oct 30, 2021

phw left a comment

zas commented Oct 31, 2021

kepstin commented Oct 31, 2021 •

edited

Loading

phw commented Nov 1, 2021

phw Nov 1, 2021

phw Nov 1, 2021

phw Nov 1, 2021

zas commented Nov 1, 2021

kepstin commented Nov 1, 2021 •

edited

Loading

PICARD-1001 Rework local cover art handling #1934

Are you sure you want to change the base?

PICARD-1001 Rework local cover art handling #1934

Conversation

kepstin commented Oct 30, 2021 • edited Loading

Summary

Problem

Solution

Action

kepstin commented Oct 30, 2021

kepstin commented Oct 30, 2021 • edited Loading

kepstin commented Oct 30, 2021

kepstin commented Oct 30, 2021

phw left a comment

Choose a reason for hiding this comment

zas commented Oct 31, 2021

kepstin commented Oct 31, 2021 • edited Loading

phw commented Nov 1, 2021

phw Nov 1, 2021

Choose a reason for hiding this comment

phw Nov 1, 2021

Choose a reason for hiding this comment

phw Nov 1, 2021

Choose a reason for hiding this comment

zas commented Nov 1, 2021

kepstin commented Nov 1, 2021 • edited Loading

kepstin commented Oct 30, 2021 •

edited

Loading

kepstin commented Oct 30, 2021 •

edited

Loading

kepstin commented Oct 31, 2021 •

edited

Loading

kepstin commented Nov 1, 2021 •

edited

Loading